-
Notifications
You must be signed in to change notification settings - Fork 34
feat: android aab signing #593
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: android aab signing #593
Conversation
🦋 Changeset detectedLatest commit: 321b308 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@mlisikbf is attempting to deploy a commit to the Callstack Team on Vercel. A member of the Team first needs to authorize it. |
| function isAab(filePath: string): boolean { | ||
| return path.extname(filePath).toLowerCase() === '.aab'; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opted to switch on path extension rather than adding a separate --aab flag, as that should be enough to distinguish and the flag seemed superfluous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense 👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run prettier here and there (this one is missing trailing newline)
|
It's on my list, will come back to this shortly! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks for the contribution! Just the few comments to be resolved + I think we should decide to go with apksigner instead of jarsigner to make it more secure & follow Android docs' recommendations.
packages/platform-android/src/lib/commands/signAndroid/signAndroid.ts
Outdated
Show resolved
Hide resolved
packages/platform-android/src/lib/commands/signAndroid/signAndroid.ts
Outdated
Show resolved
Hide resolved
packages/platform-android/src/lib/commands/signAndroid/signAndroid.ts
Outdated
Show resolved
Hide resolved
| // 6. Align archive after signing if aab | ||
| if (isAab(outputPath)) { | ||
| await alignArchive() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The post-signing alignment for AABs also overwrites the signed outputPath with the unsigned tempArchivePath, losing the signature
This bot's comment is actually incorrect, we can ignore that (LLM trash as usual). The issue is exactly why apksigner is preferred over jarsigner, because jarsigner means Android v1 signature. apksigner uses v2,3 or 4 signing which is whole-file signing and therefore there is no way to tamper with the file after signing and therefore signing is needed after alignment. However, if we move to apksigner (as I suggested in a comment below), we would then have to keep in mind the (only) right way would be align-then-sign.
also, can you send me some materials on why AAB needs aligning after signing, while APK does it before?
It's not that they need it that way, it results from the tool choice. Jarsigner (legacy, which constitutes the old Android singing scheme v1) only signs parts of the ZIP (APK / AAB), and the signatures don't cover ZIP metadata. Zipalign stores the actual alignment information in the ZIP metdata (not covered by the signatures) and this is why the signing should occur after alignment.
Apksigner signs according to v2+ Android signing schemes which treat the whole file as a blob (including ZIP metadata). Therefore, realigning means invalidating the signature.
Some information on that can be found here: https://source.android.com/docs/security/features/apksigning#v1
packages/platform-android/src/lib/commands/signAndroid/command.ts
Outdated
Show resolved
Hide resolved
packages/platform-android/src/lib/commands/signAndroid/signAndroid.ts
Outdated
Show resolved
Hide resolved
packages/platform-android/src/lib/commands/signAndroid/signAndroid.ts
Outdated
Show resolved
Hide resolved
|
Minor tweaks and we're good to merge this today |
…roid.ts Co-authored-by: Michał Pierzchała <[email protected]>
* feat: android aab signing * feat: android aab signing - docs update * feat: android aab signing - fixes assets path * feat: android aab signing - changeset * feat: android aab signing - missing semicolon * feat: android aab signing - ensures non-empty alias * feat: android aab signing - avoids shadowing node:path * feat: android aab signing - strips jarsigner password input * feat: android aab signing - corrects key-alias message * feat: android aab signing - uses input path for isAab checks * feat: android aab signing - updates changeset * feat: android aab signing - adds note on sign/align sequence * feat: android aab signing - removes aab-specific signing flow * feat: android aab signing - minimizes changes * feat: android aab signing - adds --min-sdk-version arg * feat: android aab signing - adds --min-sdk-version note in the docs * Update packages/platform-android/src/lib/commands/signAndroid/signAndroid.ts Co-authored-by: Michał Pierzchała <[email protected]> * feat: android aab signing - adds applies prettier * feat: android aab signing - moves doc comment * feat: android aab signing - removes reduntant brace * feat: android aab signing - adds comment on default --min-sdk-version * feat: android aab signing - removes --new-sdk-version from sign command --------- Co-authored-by: Michał Pierzchała <[email protected]>
Summary
Introduces aab signing using the existing
yarn sign:androidcommand, with the same list of arguments, just provide a path to an .aab file.signAndroidwill check file extension to decide on:Includes naming changes, updates to messages and docs where appropriate.
An alternative would be for end-users to invoke jarsigner themselves, extract and modify the bundle as needed.
Test plan
(cd packages/platform-android && npm link)npm rock create(select android, skip plugins)npm link @rock-js/platform-androidnpm rock create-keystore:android- use default values, for simplicity usefake-passwhen prompted for passwordnpx rock build:android --aab --local --variant release(note: using--localto opt out of cache as it does not seem to catch js changes for release variants)<WelcomeScreen/>with custom jsx)npx rock bundle --platform android --entry-file index.js --bundle-output output.bundlenpx rock sign:android ./android/app/build/outputs/bundle/release/app-release.aab --keystore ./android/app/release.keystore --keystore-password fake-pass --key-password fake-pass --key-alias rock-alias --jsbundle ./output.bundlebundletool--brew install bundletoolthenbundletool build-apks --bundle=./android/app/build/outputs/bundle/release/app-release.aab --output=output/apks.apks --mode=universal --local-testing --ks=./android/app/release.keystore --ks-key-alias=rock-alias --ks-pass=pass:fake-pass --key-pass=pass:fake-passunzip output/apks.apks -d outputand install on a connected device withadb install output/universal.apkrevert changes and verify apk signing:
npx rock build:android --local --variant releasecloses #588
Note
Generalizes Android signing to handle both APK and AAB, updating args, signing/align flow, bundle replacement paths, and docs.
@rock-js/platform-android)sign:android: Now signs bothAPKandAABbased on file extension.apksignerfor APK andjarsignerfor AAB; adjusts zipalign order accordingly.assets(APK) orbase/assets(AAB).binaryPaththroughout (apk->binaryPath/path) and updates messages.alignArchiveFile,signAab,signApk,isAab, and password handling forjarsigner.website/src/docs/cli.mdforsign:androidto acceptAPKorAABand reflect newbinaryPathargument and output descriptions.@rock-js/platform-android.Written by Cursor Bugbot for commit 321b308. This will update automatically on new commits. Configure here.